Skip to content

record security exceptions in resolved index expressions #135630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

record security exceptions in resolved index expressions #135630

wants to merge 5 commits into from
+160 −9

Conversation

richard-dennehy
Copy link
Contributor

Record security exception for resolved index expressions with CONCRETE_RESOURCE_UNAUTHORIZED results

@richard-dennehy richard-dennehy added >non-issue :Security/Security Security issues without another label Team:Security Meta label for security team labels Sep 29, 2025
@richard-dennehy richard-dennehy marked this pull request as ready for review September 29, 2025 16:15
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

n1v0lg
n1v0lg reviewed Oct 7, 2025
View reviewed changes
...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java Outdated
if (e instanceof IndexNotFoundException) {
listener.onFailure(e);
} else {
listener.onFailure(actionDenied(authentication, authzInfo, action, request, e));
Copy link
Contributor

@n1v0lg n1v0lg Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set security exceptions in the case where the action actually succeeded (due to ignore_unavailable=true) instead of when it failed. When it failed, the recorded expressions are irrelevant since we will throw directly. So we need to take the request post resolution and see if:

  1. it has resolved expressions recorded
  2. if any of those have CONCRETE_RESOURCE_UNAUTHORIZED set

If the above is true, we need to set an actionDenied exception as you do below

@n1v0lg n1v0lg requested review from ywangd and removed request for ywangd October 7, 2025 16:04
@n1v0lg
Copy link
Contributor

n1v0lg commented Oct 7, 2025

Sorry, only got to this late!

@richard-dennehy could you update the branch, address this bit, and the req a review from @ywangd when it's ready? If my comment doesn't make sense I hope Yang can provide guidance. The Jira, for reference is ES-13005.

@ywangd
Copy link
Member

ywangd commented Oct 8, 2025

I am bit unsure about the need of this change. Is there a JIRA ticket for it that might provide some more context.

We record CONCRETE_RESOURCE_UNAUTHORIZED with null exception in IndexAbstractionResolver when the index is unauthorized. In CrossProjectIndexResolutionValidator, we check and create a security exception for them. So it already works as is today. Is the problem that this exception does not have sufficient information to be user-friendly and we want something like the exception created by AuthorizationService#actionDenied?

@richard-dennehy
Copy link
Contributor Author

Nikolaj mentioned the Jira ticket in his last comment: ES-13005

But there's not a great deal of context provided there, tbh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n1v0lg n1v0lg n1v0lg left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
>non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v9.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@richard-dennehy @elasticsearchmachine @n1v0lg @ywangd